-
Notifications
You must be signed in to change notification settings - Fork 528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Arduino] Add an example to use Subscriber and ServiceServer in a class #321
[Arduino] Add an example to use Subscriber and ServiceServer in a class #321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick question about stack v. heap initialization, but overall this is great— thanks for contributing it!
Blinker(byte pin, uint16_t period) | ||
: pin_(pin), period_(period), | ||
subscriber_("set_blink_period", &Blinker::set_period_callback, this), | ||
service_server_("activate_blinker", &Blinker::service_callback, this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be slightly clearer to have these members as pointers and then initialize the objects in the constructor, but I'm not married to having it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, I should do dynamic memory allocation on a microcontroller? I am not a fan of that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a one-time allocation at startup, so that's pretty safe, but if you'd rather not, it can stay as-is. Another possibility would be adding do-nothing constructors to the classes so that they can be initialized empty and the reinitialized afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the do-nothing constructor path. But I don't get why you want to initialize the class later? In the setup()
function to avoid the init()
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I'm suggesting is having the option for the example to look like this, which feels a bit more like idiomatic roscpp:
Blinker(byte pin, uint16_t period) : pin_(pin), period_(period)
{
subscriber_ = ros::Subscriber("set_blink_period", &Blinker::set_period_callback, this),
service_server_ = ros::ServiceServer("activate_blinker", &Blinker::service_callback, this)
}
void init(ros::NodeHandle& nh)
{
pinMode(pin_, OUTPUT);
nh.subscribe(subscriber_);
nh.advertiseService(service_server_);
}
However, it's not going to work regardless without also re-specifying all the template arguments since there's no type inference on constructors, so that kind of blows it away anyway (and there'd need to be copy constructors added). So I think the best thing is to merge this contribution as-is, and if there's interest now or later in streamlining or adding to the API, that can be discussed on separate PR.
Following #282 I have added an example on how to use a Subscriber and a ServiceService as class members.
It was not documented before so there is a question on answers.ros.org about that subject: https://answers.ros.org/question/258249/publisher-or-subscriber-inside-a-class-rosserial/